[improvement](hive) Batch Hive metastore partition access - #67186
[improvement](hive) Batch Hive metastore partition access#67186CalvinKirs wants to merge 29 commits into
Conversation
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
morrySnow
changed the title
Issue Number: None
Related PR: None
Problem Summary: Hive tables with very large partition counts could issue one HMS RPC per partition or one unbounded getPartitionsByNames request. This caused excessive serial RPC latency and exposed Thrift message-size, timeout, memory, cancellation, cache-concurrency, and observability gaps. Add one HMS-owned bounded batch loader with adaptive halving for classified Thrift failures, strict result-integrity validation, request deadline and cancellation, physical RPC metrics, and Query Profile events. Reuse it from query, MTMV, statistics, display, and regular write partition-object paths, and coalesce concurrent cache misses per partition with bounded FIFO load slots.
Hive metastore partition-object access is now bounded, cancellable, observable, and shared across query, MTMV, statistics, display, and write paths.
- Test: Unit Test
- HMS batch, cache, and pool tests
- Hive metadata and transaction tests
- MTMV and connector profile tests
- Behavior changed: Yes (large Hive partition-object reads use bounded HMS batches with adaptive fallback and strict integrity checks)
- Does this need documentation: No
00588b2 to
7ab8c53
Compare
|
run buildall |
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Requesting changes. Three review rounds converged cleanly—all final-round reviewers returned NO_NEW_VALUABLE_FINDINGS—on nine distinct issues: five P1 correctness/compatibility/capacity problems and four P2 lifecycle/observability problems. The review is complete rather than capped.
Critical checkpoint conclusions:
- Goal and proof: the PR establishes the intended common HMS batching, integrity, cancellation, cache-coordination, and telemetry path, and the added unit tests prove many normal/error paths. The nine inline defects prevent the implementation from safely meeting the full contract.
- Scope and clarity: this is a justified but broad 58-file cross-cutting change. The raw-loader/cache/SPI decomposition is generally clear; the remaining defects concentrate at ownership and phase boundaries.
- Concurrency: query/MTMV/display request threads coordinate through per-key in-flight state, FIFO slots, and striped locks while HMS RPCs stay outside locks. Normal owner/waiter cleanup and lock ordering are sound, but refresh can miss a newly registered publisher, cache-disabled traffic bypasses admission, and MTMV local mapping/version capture is not atomic.
- Lifecycle/static state: task-owned
StatementContextcleanup and successful connector replacement are sound. Temporary validation contexts and superseded failed-init contexts leak shared metrics references. No C++ static-initialization concern applies. - Configuration: the new HMS batch/fallback properties are validated and wired consistently through Hive and Hudi; they are catalog-scoped rather than dynamic process variables. Disabling partition retention incorrectly disables the pool-derived admission bound.
- Compatibility: the public connector SPI surface changes without the repository-mandated 7.0 major bump, so the major-only plugin gate permits incompatible 6.x linkage. No storage-format or FE/BE protocol change is present.
- Parallel paths: all eight production partition-object callers and Hive/Hudi construction were traced. Query/write/statistics sources and sessions are propagated; SHOW/display purpose is not, and the analogous task/display MTMV phase split needs the same ownership fix.
- Conditions and error handling: strict identity/order validation, typed integrity failures, statement deadlines, and cancellation/pool-taint boundaries are otherwise sound. The generic
TTransportExceptioncondition is too broad, and the MTMV outside-lock predicate is too broad. - Tests and results: changed tests cover batching, fallback state, result integrity, owner/waiter cleanup, cancellation, metrics/profile aggregation, and MTMV bulk caching. Missing decisive coverage is called out inline for refresh registration, disabled-cache zero-pool concurrency, mixed/cloud mapping races, context ownership, DISPLAY, outer logical-event aggregation, and process wait metrics. Per the review prompt, I ran no build or tests. Current CI has green style/security/coverage contexts; FE UT, compile, performance, and automated review are still pending.
- Observability: event schema/cardinality and profile serialization are generally sound, but context leaks, DISPLAY mislabelling, caller-event misaggregation, and missing process wait metrics make the advertised telemetry inaccurate.
- Transactions, persistence, writes, and FE/BE variables: no transaction protocol, persisted schema, EditLog payload, storage format, or FE-to-BE variable was added. Replay initialization and Hive write/transaction callers were audited; their accepted issue is the shared context lifecycle leak, not transaction atomicity.
- Performance: bounded physical batches and single-flight improve the intended large-partition path, but ordinary outages can be amplified through the halving ladder and cache-disabled zero-pool traffic can open unbounded concurrent clients.
- Other applicable checks: this PR is FE Java only, so BE memory tracking, nullable-column handling, delete bitmaps, and C++ static/TU rules are not applicable. No additional issue survived the final changed-file and missed-path sweep.
No additional user-provided review focus was supplied.
| this.rawStoragePropsSupplier = | ||
| Objects.requireNonNull(rawStoragePropsSupplier, "rawStoragePropsSupplier"); | ||
| this.environment = buildEnvironment(); | ||
| this.metadataAccessMetrics = new ConnectorMetadataAccessMetrics(catalogName); |
There was a problem hiding this comment.
[P2] Give the validation metrics reference an owner. This constructor now acquires a shared per-catalog metrics reference even for the temporary context created by forCatalogCreationValidation(). CatalogFactory passes that context inline and retains only the connector, while PluginDrivenExternalCatalog explicitly leaves connectorContext null for this validation connector, so neither initialization nor catalog teardown can call DefaultConnectorContext.close() on it. Each create/replay attempt therefore leaves an entry in SHARED_METRICS; after the live catalog records metrics, DROP also cannot unregister those catalog-labelled series because the leaked reference keeps the count nonzero. Please make validation use a non-acquiring metrics sink or give the temporary context an explicit owner that closes it on every success/failure/fallback path. The same ownership rule is also needed for live initialization: construct into a local context, publish it only after connector creation succeeds, and close it on null/throw so repeated retries cannot overwrite and leak failed contexts.
| } | ||
| // Write binding gained execution-capability methods in this surface revision. A plugin built against | ||
| // major 5 must be refused rather than run against a contract it did not compile against. | ||
| // Write binding gained execution-capability methods, while metadata access gained operation control, |
There was a problem hiding this comment.
[P1] Bump the connector SPI major for this surface change. This PR adds methods and types to the public connector SPI, but the API is still stamped as 6.0. The policy beside connector.plugin.api.version requires a same-commit major bump for any SPI surface addition, and ApiVersionGate checks only major equality. As written, a plugin compiled against these new APIs is labelled 6.0 and can be accepted by an older 6.0 FE, then fail at first use with NoSuchMethodError/NoClassDefFoundError. Please bump the connector API major (and this assertion) to 7.0 in this commit.
There was a problem hiding this comment.
Thanks for calling this out. We intentionally will not bump the connector SPI to 7.0 in this PR. The compatibility boundary should be a published connector API version, not every PR that evolves an API which is still unreleased on master. Connector SPI 6.0 was introduced on master on Aug 17, 2026, and no release tag contains that commit, so 6.0 is still the next unpublished surface; this PR updates that same pre-release surface and its frozen baseline before publication. Once 6.0 is released, a subsequent incompatible surface change must bump the major. Bumping the major once per pre-release PR would consume versions without creating a real artifact compatibility boundary. The existing major gate still correctly separates published/older major 5 plugins from the upcoming major 6 API.
| try { | ||
| invalidateInFlightPartitionLoads(key -> key.matches(dbName, tableName), true); | ||
| } finally { | ||
| stateLock.unlock(); |
There was a problem hiding this comment.
[P1] Keep registration fenced through the cache clear. The state lock is released before partitionsCache.invalidateIf() bumps the generation. A cold request can therefore register after the in-flight scan, start its HMS RPC, then let this refresh clear the cache and return; because that new batch was never marked invalid and publishOwnedPartitions() uses a direct put, its pre-clear load is cached afterward for the full TTL. The same gap exists in partition/DB/catalog invalidation. Please perform the matching cache invalidation under the same stripe(s), or capture/check a refresh epoch at owner publication, and add the mark/register/clear/publish interleaving to the concurrency tests.
| } | ||
| for (Throwable current = failure.getCause(); current != null; current = current.getCause()) { | ||
| String className = current.getClass().getName(); | ||
| if (className.endsWith(".TTransportException")) { |
There was a problem hiding this comment.
[P1] Do not halve batches for every transport outage. This class-name check makes a closed/refused/reset/EOF/timeout TTransportException degradable even though reducing the payload cannot repair the connection. With the defaults, one 5,000-name offset can be replayed 13 times down to size 1 within the 30-second budget, and each logical call sits above Hive's own retry/reconnect proxy and may create/taint another client. That amplifies an HMS outage precisely while it is unhealthy. Please restrict fallback to explicit frame/message/request/partition-limit signals (or a proven oversize transport code), and make ordinary transport failures terminate after the original logical attempt.
| int start = 0; | ||
| private void loadMissingPartitions(HmsPartitionRequest request, List<String> initialMissNames, | ||
| Map<List<String>, HmsPartitionInfo> resultByIdentity) { | ||
| if (!partitionsCache.isEffectiveEnabled()) { |
There was a problem hiding this comment.
[P1] Preserve load admission when partition caching is disabled. This early return skips both single-flight retention and the new window/slot limiter. In the supported hive.metastore.client.pool.size=0 configuration, the constructor deliberately converts zero to one cold-load slot, but every disabled-cache request now bypasses that slot and ThriftHmsClient creates a fresh client per call; N concurrent scans/freshness probes can therefore open N HMS connections. Please keep windowing and slot admission on this path while skipping only cache coordination/publication, and cover zero-pool plus disabled cache concurrently.
| MTMVPartitionUtil.addPartition(mtmv, partitionKeyDesc); | ||
| } | ||
| } | ||
| boolean buildContextUnderLock = Config.isNotCloudMode() |
There was a problem hiding this comment.
[P1] Keep local PCT mappings atomic with their versions in mixed MVs. This condition moves the entire context build outside the sorted table locks whenever any base table is MVCC. If the actual PCT table is a local OlapTable, its mapping is copied here at T1, external preload can then block, and the later locked refreshLocalBaseVersions() refreshes only versions—not partitionMappings. A local partition dropped in that window remains in the mapping and makes the locked version lookup fail; an added partition is omitted from comparison/refresh. The base code built both together under the locks. Please split the capture so external pins/I/O stay outside, while local PCT mappings and versions are rebuilt together under the sorted FE locks. Cloud local-only plans also always take this branch and the refresh helper is a no-op there, so preserve an atomic cloud capture as well. Apply the same fix to the analogous PartitionsProcDir branch and add mixed local-PCT/external-MVCC plus cloud local-only race tests.
| } | ||
| HiveTableHandle hiveHandle = (HiveTableHandle) handle; | ||
| List<HmsPartitionInfo> partitions = hmsClient.getPartitions( | ||
| session, HmsPartitionAccessSource.MTMV, |
There was a problem hiding this comment.
[P2] Preserve the display source in freshness telemetry. SHOW PARTITIONS now builds and preloads MTMVRefreshContext, reaches these freshness methods, and is always emitted as MTMV here; the sibling whole-table freshness call is hard-coded the same way. There is no production use of the newly added HmsPartitionAccessSource.DISPLAY, so display traffic is indistinguishable from refresh/rewrite work in both process metrics and Query Profile despite the per-source observability contract. Please thread the logical access purpose into this freshness request and emit DISPLAY for the proc/display path, with a production-chain test.
| request, initialMissNames, partitionsCache.invalidationGeneration(), resultByIdentity); | ||
| return; | ||
| } | ||
| for (int offset = 0; offset < initialMissNames.size(); offset += partitionLoadWindowSize) { |
There was a problem hiding this comment.
[P2] Emit one logical event for the caller's request. The cache splits one business request into partitionLoadWindowSize windows and each copied request invokes the raw loader, whose finally records a completed logical event. A cold 12,000-name call therefore increments LogicalRequests three times; if the third window fails, the single caller-visible request is reported as two successes plus one failure. The cache-disabled path reports the same call once, so success rates and requested-item metrics depend on cache configuration rather than business semantics. All-hit and pure-waiter calls emit no completed business event, and mixed hit/miss calls report only owner misses. Please move event ownership to the outer cache call, aggregate the shared business-request state, and emit exactly once while retaining physical-attempt counters; test all-hit, mixed, pure-waiter, and multi-window later-failure cases.
| .success(success) | ||
| .build(); | ||
| try { | ||
| request.getMetadataAccessObserver().record(event); |
There was a problem hiding this comment.
[P2] Publish coordination waits to process metrics too. Both in-flight and cold-load-slot wait events are recorded only through the request observer, which is a Query Profile sink and becomes NOOP when profiling is disabled. The catalog observer used by the raw loader never reaches this cache layer, so FE metrics permanently omit both newly advertised wait operations even while reporting their downstream HMS call. Please give the cache the catalog observer and safely publish these events to both sinks, with process-metric coverage when query profiling is disabled.
TPC-H: Total hot run time: 16850 ms |
TPC-DS: Total hot run time: 81114 ms |
ClickBench: Total hot run time: 14.48 s |
FE UT Coverage ReportIncrement line coverage |
|
/review |
There was a problem hiding this comment.
Request changes. Three review rounds converged with four additional issues: one P1 cancellation/lifecycle gap and three P2 compatibility/performance gaps. I did not duplicate the eight existing issue threads; their unresolved blockers remain part of this verdict. The live PR still matches the authoritative bundle at head 7ab8c5329b97a6ac3e9361e593383e5ac0bf6bd8. Current CI has compile, FE UT, CheckStyle, P0, non-concurrent, vault, and performance checks passing, while External Regression and cloud_p0 are failing.
Critical checkpoint conclusions:
- Goal and proof: The PR implements batched/cached HMS partition freshness, cooperative cancellation, telemetry, and MTMV bulk snapshot loading, and its unit tests prove many component paths. It does not fully achieve the stated bounded/large-scale behavior because client construction remains outside cancellation/deadline control, the cache-backed scale path repeats canonicalization, and MTMV can eagerly load a huge union before a locally decisive stale gate.
- Scope and focus: The 58-file connector/HMS/MTMV change is internally related but not yet safely mergeable. The user focus file contained no additional focus request; the full PR was reviewed.
- Concurrency and thread safety: Enabled-cache owner/waiter futures, permits, publication, retry cleanup, and lock ordering otherwise balance. Existing threads already cover the cache-invalidation fence and disabled-cache admission bypass; the new P1 below covers synchronous client creation before cancellation can act. Heavy external work is generally moved outside FE locks, subject to the existing mixed local/cloud atomicity thread.
- Error handling: Strict result-integrity failures and cancellation propagation are fail-loud and preserve causes in the inspected paths. The existing broad transport-fallback thread and the new eager-preload ordering can still amplify or surface avoidable HMS failures.
- Lifecycle: Watchdog ThreadLocal cleanup, interrupt ownership, pooled-client taint/return, statement pins/scopes, and normal connector-context close were traced. Existing metrics-reference ownership remains a live thread; any fix for client creation must destroy a late result after cancellation, deadline, or concurrent close.
- Configuration and dynamic behavior: Hive and Hudi bind the same positive batch/timeout properties and defaults through catalog construction/replay. No additional dynamic-update divergence survived review.
- Compatibility and rolling upgrade: Default SPI methods preserve old implementation linkage, and the existing API-major thread includes the unreleased-6.0 context. Separately, the frozen-surface test omits the new reachable session/control/observer/event/abort contracts and metadata return types, so future incompatible drift can evade the gate.
- Parallel paths: Query, statistics, MTMV, and write callers plus Hive/Hudi construction were checked. Rewrite, task, metadata/global sync, and proc/display MTMV paths were all traced. The existing DISPLAY-source thread remains the only distinct source-label issue.
- Special conditionals: Excluded-table and PCT-first comparison semantics are intentional. Existing review context covers transport degradability and cache-disabled branching; the new MTMV finding covers preload ordering before the name-set condition.
- Test coverage: Added tests cover batching, strict ordering, cache coordination, pool wait cancellation, metrics/profile aggregation, context capture, and 160k aggregation. Missing cases are identified inline: blocking client creation, frozen reachable SPI contracts, cache-backed parse counts, and large name-set mismatch with zero freshness calls.
- Test results: This review-only environment expressly prohibited builds/tests, so none were run here. No regression
.outfiles changed. Live FE UT/compile/style checks pass, butExternal Regressionandcloud_p0currently fail. - Observability: Process metrics and Query Profile coverage were inspected. Existing threads cover metric reference ownership, fragmented logical events, missing process wait metrics, and DISPLAY attribution; no additional observability issue survived.
- Transaction and persistence: MTMV refresh snapshot generation, manual/COMPLETE refresh, current-relation resolution, and per-partition persistence inputs were traced. No new EditLog schema is introduced; the existing MTMV mapping/version atomicity thread remains applicable.
- Data writes and crash behavior: No new BE/storage data-write path is introduced. MTMV refresh scheduling and snapshot capture were checked; no distinct crash leak or partial-write issue survived beyond the live atomicity/lifecycle threads.
- FE/BE variables: No new FE-to-BE variable or protocol field is introduced.
- Memory safety and nullable handling: The change is Java/FE-only; BE allocator, C++ lifetime, and nullable-column checkpoints are not applicable. Java ownership and large temporary allocations were reviewed, with the repeated identity allocation issue called out inline.
- Data correctness: Strict partition identity, duplicate, missing, unexpected, and ordering checks are coherent. Existing threads cover cache freshness fencing and MTMV atomicity; the dismissed display snapshot split predates this PR.
- Performance: Batching removes per-partition RPCs, but the cache-backed request performs
3Nparses on all hits and6N+Cwhen fully cold, and MTMV may issue a 160k-name freshness load before a set mismatch already proves staleness. - Other issues and completion: All candidates are accepted, deduplicated, or dismissed with code evidence. Round 3 ended with
NO_NEW_VALUABLE_FINDINGSfrom both normal full reviews and the independent risk review, so this review is complete.
| waitMillis = Math.min(waitMillis, operationRemainingMillis); | ||
| } | ||
| try { | ||
| return clientPool.borrowObject(waitMillis); |
There was a problem hiding this comment.
[P1] Bound HMS client creation with the operation control. On an empty pool, Commons Pool 2.2 runs HmsClientFactory.create() synchronously inside borrowObject(waitMillis) before the timed idle-object wait, so waitMillis does not bound createFreshClient(); the pool-disabled branch calls it directly as well. Kerberos login, DNS, or socket construction can therefore remain stuck after KILL/deadline, before HmsRemoteCallTracking installs its watchdog and before the next checkActive(). Please make creation cancellable/deadline-aware (and destroy any client that completes late) for both branches, with blocking-provider KILL/deadline tests.
| return ConnectorStatementScope.NONE; | ||
| } | ||
|
|
||
| /** Returns cooperative cancellation and deadline control for connector metadata operations. */ |
There was a problem hiding this comment.
[P2] Freeze the new session/control API in the plugin surface. ConnectorPluginSurfaceTest.FROZEN_TYPES does not include ConnectorSession or the new control/observer/event/abort types, so the regenerated baseline records ConnectorContext#getMetadataAccessObserver() but not these two session methods or the callable contracts they expose. The separate metadata baseline also omits return types. That leaves later removal/re-signing of this new 6.0 surface invisible to the stated compatibility speed bump. This is independent of whether 6.0 is still unpublished: please freeze these reachable SPI types (or recursively freeze reachable SPI contracts), regenerate the baseline, and assert the new methods are present.
| operationControl.checkActive(); | ||
| } | ||
| String partitionName = partitionNames.get(i); | ||
| HmsPartitionIdentity.ParsedPartitionName parsed = HmsPartitionIdentity.parse(partitionName); |
There was a problem hiding this comment.
[P2] Retain parsed identities across the cache-backed request. This builder validates every partition name with HmsPartitionIdentity.parse() and then discards the result. The normal cold-cache path reparses all names during cache lookup, registration, copied-window construction, raw validation, and final reconstruction—6N+C parse/unescape passes for a fully cold sole owner (roughly 720k canonicalizations for 120k names); even all hits take 3N. The added 120k test covers only the raw loader, so it misses this decorator cost. Please carry immutable parsed keys/values on the request and share/slice them through windows and validation, with a cache-backed large-request parse-count/performance test.
| throws AnalysisException { | ||
| MTMV mtmv = context.getMtmv(); | ||
| Set<String> partitionNames = mtmv.getPartitionNames(); | ||
| context.preloadSnapshots(partitionNames, tables, excludeTables); |
There was a problem hiding this comment.
[P2] Check the persisted partition set before eager freshness loading. This preload fetches partition-detail freshness for the entire mapped PCT union before isSyncWithPartitions() compares the current names with the persisted getPctSnapshots() set. With a coarse mapping over 160k Hive partitions, adding or dropping one partition already proves the MTMV stale locally, but this order now issues the full HMS freshness request first (and can fail before returning that deterministic stale result); the pre-PR path performed the set gate before fetching snapshots. Please make the comparison two-phase—reject name-set mismatches first, then preload only mappings that still need version checks—and add large mismatch coverage asserting no partition-freshness call.
|
/review |
There was a problem hiding this comment.
Reviewed the authoritative 58-file bundle and all live/raw review comments through three convergence rounds. Two additional non-duplicate issues remain, alongside the unresolved live blockers, so I am requesting changes.
Key checkpoints: the cross-layer scope matches the stated bounded/cancellable HMS-load and MTMV-freshness goal, and the new request, loader, control, telemetry, and refresh-context helpers generally keep responsibilities clear. Strict response validation/order, typed cancellation, cache/pool cleanup, connector construction, statement-scope closure, and the main query/statistics/scan/write/refresh/rewrite/display paths were traced. The remaining new gaps are (1) rewrite eagerly loading locally rejectable candidates while planner locks are held and (2) equivalent waiters serially replaying a shared integrity failure. Existing threads already fence the other identified concurrency, invalidation, compatibility, metrics, and eager-loading concerns. Hive/Hudi configuration validation and SPI forwarding were checked; this patch changes no storage/EditLog format, transaction protocol, FE/BE wire value, or BE memory/nullability path.
No local build or tests were run because the review bundle forbids them. Current checks show FE UT, compile, P0, nonconcurrent, and vault passing; External Regression and cloud_p0 are failing, with no public failure detail available from the linked TeamCity endpoints. The changed unit tests cover most raw/cache/control and MTMV batch paths, but not the two concurrent/production-chain cases called out inline.
### What problem does this PR solve? Issue Number: None Related PR: apache#67186 Problem Summary: The shared Hive metastore partition batch path still had lifecycle, concurrency, compatibility, and MTMV freshness edge cases found during review. Blocking HMS client construction could outlive cancellation, parsed identities were repeatedly rebuilt in cache windows, equivalent waiters could replay terminal integrity failures, and rewrite or refresh paths could preload metadata before deterministic stale gates. This change makes client creation operation-controlled, reuses immutable parsed identities, scopes waiter retries, applies persisted-set gates before freshness loading, and completes SPI and metrics lifecycle coverage. ### Release note Hive metastore partition batching now has stronger cancellation, concurrent failure sharing, MTMV freshness gating, and connector SPI compatibility checks. ### Check List (For Author) - Test: Unit Test - Targeted FE unit tests for HMS cache concurrency, pool control, batch loading, MTMV rewrite and refresh, metrics, and Connector SPI surface - Behavior changed: Yes. Equivalent cache waiters share terminal integrity failures, narrower waiters retry independently, and MTMV rewrite skips freshness loads for locally rejected candidates. - Does this need documentation: No
### What problem does this PR solve? Issue Number: None Related PR: apache#67186 Problem Summary: The new connector metadata access source import was placed before lexicographically earlier HMS and connector SPI imports in three Hive connector classes, causing the full FE CheckStyle workflow to fail. Reorder those imports according to the Doris CustomImportOrder rule. ### Release note None ### Check List (For Author) - Test: Unit Test - cd fe && mvn clean checkstyle:check - Behavior changed: No - Does this need documentation: No
|
/review |
|
Codex automated review failed and did not complete. Error: Your access token could not be refreshed because your refresh token was revoked. Please log out and sign in again. Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
|
run buildall |
|
/review |
There was a problem hiding this comment.
Requesting changes for four distinct current-head issues: unbounded outstanding HMS client creators, MTMV persistence losing union batching on first/incomplete or mapping-changed baselines, cloud version RPCs under FE table locks, and nested wait spans inflating the legacy metadata-time total.
Checkpoint conclusions:
- Goal, focus, and scope: the common bounded HMS path is broadly wired across query, statistics, MTMV, display, transaction, and write callers; no extra user focus was supplied. The first-refresh persistence case still misses the central batching goal.
- Correctness, conditions, and parallel paths: strict identity/order validation and migrated Hive/Hudi caller paths were checked, including cache-enabled/disabled, pooled/unpooled, first/incomplete/changed MTMV snapshots, local/external/cloud, and success/failure/cancellation branches.
- Concurrency and lifecycle: cache owner/waiter, invalidation, slot, retry, and publication paths were reviewed. The asynchronous creation fix has no bound on interrupt-ignoring creator tasks.
- Configuration and compatibility: new property defaults/validation, source/control defaults, connector SPI freezes, and the unreleased-6.0 compatibility context were checked. No new FE-BE wire dependency was introduced.
- Observability and performance: detailed operation counters remain useful, but the legacy query total double-counts nested waits. The first/incomplete MTMV path can turn a 160k disjoint mapping into roughly 160k logical one-name freshness requests.
- Transactions, persistence, writes, and atomicity: transaction/write callers use the common API; snapshot persistence has the separate preload-mode bug below, and cloud recapture performs remote work inside metadata locks. Existing live atomicity threads were treated as duplicate fences.
- Tests/results: reviewed the changed unit tests and the PR's reported 250-partition manual profile. No build or test command was run in this review, as required by the review task; the PR also states the 120k end-to-end case was not rerun. Missing focused coverage is called out inline.
A complete 63-file final sweep and a second full convergence round found no additional distinct issues beyond these four and existing review threads.
TPC-H: Total hot run time: 17284 ms |
TPC-DS: Total hot run time: 83713 ms |
ClickBench: Total hot run time: 14.96 s |
FE UT Coverage ReportIncrement line coverage |
### What problem does this PR solve? Issue Number: None Related PR: apache#67186 Problem Summary: Review found four remaining correctness and resource-control gaps in the shared HMS partition batch work. Nested coordination spans double-counted legacy profile time, non-cooperative HMS client creation could grow unbounded daemon threads, MTMV snapshot persistence skipped union preloading for first or changed baselines, and cloud MTMV version refreshes could issue remote calls while FE table locks were held. Count only top-level HMS access in legacy totals, retain bounded client-creation admission until real creator exit, split persistence preloading from comparison gating, and preload cloud MTMV versions before locks while revalidating locked structure from raw local caches. ### Release note Improve HMS partition batch profiling, cancellation resource bounds, MTMV snapshot batching, and cloud lock behavior. ### Check List (For Author) - Test: Unit Test - SummaryProfileTest, ThriftHmsClientPoolControlTest, MTMVRefreshContextBatchTest, MTMVPartitionUtilTest, MTMVRewriteUtilTest, PreloadExternalMetadataTest (46 tests) - MTMVTaskTest and PartitionsProcDirTest (18 tests) - cd fe && mvn clean checkstyle:check - Behavior changed: Yes. Legacy profile totals no longer double-count nested waits, HMS creators are bounded, persistence uses ungated union preloading, and cloud version RPCs run before FE table locks. - Does this need documentation: No
|
/review |
There was a problem hiding this comment.
Reviewed exact head 43bed7de746e1c13586452e3b22b5ea539cd9f58 against base 7219c67265bc700c1457b3339c6a9d0143d36c1f.
Requesting changes for three distinct P2 issues in the new HMS statistics, MTMV bulk-freshness, and scan-profile paths.
Critical checkpoint conclusions:
- Goal and scope: all 44 authoritative changed paths were reviewed. The patch substantially achieves bounded/adaptive partition-object reads, exact versus omission-tolerant validation, cache-safe publication, MTMV bulk reuse, and query-profile aggregation, subject to the three inline failure/observability regressions.
- Concurrency and lifecycle: cache publication/invalidation leases, pooled and pool-size-zero client retirement, per-scan synchronized aggregation, atomic submitted-task finalization, and request-scoped MTMV caches were traced. Heavy HMS/filesystem work remains outside the reviewed locks, no new lock-order cycle was found, and existing live threads fence earlier lifecycle concerns.
- Configuration and compatibility:
hive.hms_partitions_batch_size_per_rpcreaches both live Hive and Hudi client builders with consistent positive validation. The connector bulk-freshness method has a scalar default and an updated surface baseline; the prior API-major discussion is already covered by a live thread. No FE/BE protocol or storage-format change applies. - Parallel and conditional paths: exact transaction-owned reads, omission-tolerant scan/freshness/write-plan reads, raw/cached and pooled/unpooled clients, task/rewrite/display/persistence MTMV callers, synchronous/batch/prune-to-zero scans, fallback, terminal failure, and cleanup paths were checked. Remaining non-inline concerns are either disproved or duplicate-fenced.
- Tests and observability: changed tests cover large/trailing/adaptive batches, integrity and omission contracts, cache invalidation fencing, unpooled reuse/cleanup, pin/cache reuse, and provider-stage profile success/failure. They do not cover the three inline triggers: pre-wire/retry-proxy RPC counts, partial-union rewrite failure isolation, and pruning failure before provider creation. No builds or tests were run because the review prompt explicitly prohibited them; available static CI checks on this head are green.
- Persistence, transactions, and writes: no EditLog or data-write protocol change is introduced. Transaction-owned partition identities remain exact, and MTMV snapshot persistence remains strict; any rewrite-isolation repair must preserve persistence completeness.
- Performance and other correctness: remote payloads are bounded and request parsing, validation, and reconstruction remain linear. Previously reported eager/lock-held MTMV work and other profile concerns were not duplicated. No additional security, memory, configuration-observation, or material performance issue survived the final sweep.
The review converged after two rounds: every candidate was accepted, dismissed with production-path evidence, or hard-deduplicated against the live discussions. The user focus file supplied no additional focus points.
| for (HmsPartitionIdentity.ParsedPartitionName partition : batch) { | ||
| batchNames.add(partition.getName()); | ||
| } | ||
| attempts++; |
There was a problem hiding this comment.
[P2] Do not report transport invocations as physical HMS RPCs. These counters and the RPC timer start before transport.getPartitionsByNames; the pooled path can then fail in borrowClient, fresh-client creation, or outer authentication without calling HMS at all, while the default RetryingMetaStoreClient can perform multiple wire attempts inside one invocation. The Query Profile can therefore show one RpcAttempt/all RpcItems for zero wire calls, or undercount retries, and RpcElapsedTime includes setup/pool wait despite the new API documenting physical-attempt statistics. Instrument actual client attempts (including retries), or rename/separate these as batch-invocation and setup metrics, with pre-wire-failure and retry coverage.
| if (!withinGracePeriod && mtmvNeedComparePartitions.contains(candidate.getName())) { | ||
| partitionsToPreload.add(candidate.getName()); | ||
| } | ||
| } |
There was a problem hiding this comment.
[P2] Preserve per-partition rewrite failure isolation. This rewrite-wide preload sits outside the loop's existing checked-failure boundary. With mv1 -> p1 and mv2 -> p2, if p1 disappears after the pin, Hive's omission-tolerant bulk result can still contain p2, but getPartitionSnapshots throws for p1 and this catch returns before mv2 is evaluated. The previous scalar lookup threw inside the per-partition try, so only mv1 was skipped. Please retain per-name bulk outcomes (for example, defer missing-name errors to PreparedPartitionSnapshots#get) and add a two-partition pin/drop test proving the unaffected partition remains rewritable.
|
|
||
| List<HmsPartitionInfo> prunedPartitions = matchedPartNames.isEmpty() | ||
| ? Collections.emptyList() | ||
| : hmsClient.getPartitions(hiveHandle.getDbName(), |
There was a problem hiding this comment.
[P2] Preserve failed pruning stats before the scan provider exists. A selective equality-pruning request can perform HMS batches and then throw a stats-bearing HmsClientException here, before a new handle or HiveScanPlanProvider is created. convertPredicate propagates that failure, so the synchronous/batch finalizers never run and the Query Profile omits the request that aborted planning. The existing fixes cover successful pruning handoff and failures inside planScan, not this earlier boundary. Establish the scan-scoped profile owner before filter pushdown (or otherwise publish the attached stats while preserving the primary exception), and add a production-chain failing-prune test.
### What problem does this PR solve? Issue Number: None Related PR: apache#67186 Problem Summary: Concurrent cold partition scans could issue duplicate HMS loads after the cache unification, MTMV bulk snapshot preload widened one missing partition into a rewrite-wide or task-wide failure, pruning failures could lose their HMS batch profile before a scan provider existed, and profile fields described transport invocations as physical RPC attempts. Restore per-partition owner/waiter coordination with invalidation-aware bulk fences, retain MTMV snapshot outcomes per partition, transfer pruning failures through statement scope, and rename the metrics to their actual transport semantics. ### Release note HMS partition profile metrics now use TransportInvocations, TransportItems, TransportElapsedTime, and MaxTransportElapsedTime. ### Check List (For Author) - Test: Unit Test - Added concurrency, invalidation, MTMV partial-outcome, and pruning-profile handoff coverage. The focused run-fe-ut.sh execution was attempted but stopped before Java compilation because this worktree is missing thirdparty/installed/bin/protoc. - Behavior changed: Yes. Concurrent partition loads are coalesced, MTMV missing-partition failures are isolated by partition, pruning failures publish profiles, and HMS profile metric names are corrected. - Does this need documentation: No
|
/run buildall |
### What problem does this PR solve? Issue Number: None Related PR: apache#67186 Problem Summary: The MTMV partial-result change stopped constructing an ArrayList in PluginDrivenMvccExternalTable but left the import behind, causing the FE CheckStyle workflow to fail. Remove the unused import. ### Release note None ### Check List (For Author) - Test: Unit Test - mvn clean checkstyle:check (74/74 modules passed) - Behavior changed: No - Does this need documentation: No
|
/run buildall |
TPC-H: Total hot run time: 16874 ms |
TPC-DS: Total hot run time: 81557 ms |
ClickBench: Total hot run time: 14.62 s |
FE UT Coverage ReportIncrement line coverage |
FE Regression Coverage ReportIncrement line coverage |
### What problem does this PR solve? Issue Number: None Related PR: apache#67186 Problem Summary: HMS partition requests with exact and omission-tolerant contracts share per-partition in-flight loads. An omission-tolerant waiter could inherit an exact owner missing-result failure, a second-cache-check handoff could expose a batch without a bulk-load handle and dereference null after invalidation, and owner/waiter callers could race while rewriting statistics on the same shared exception. Retry shared missing-result failures under the waiter contract, make handleless handoffs invalidation-safe, and give each waiter a caller-local exception with a stable transport-stat snapshot. ### Release note None ### Check List (For Author) - Test: Unit Test - CachingHmsClientTest: 35 tests passed, including three new concurrent request-contract, invalidation, and failure-statistics cases - FE Maven CheckStyle validation passed with zero violations for all selected modules - Behavior changed: Yes. Coalesced HMS partition requests now preserve each caller contract and failure statistics under concurrency. - Does this need documentation: No
|
/run buildall |
TPC-H: Total hot run time: 16547 ms |
TPC-DS: Total hot run time: 81853 ms |
ClickBench: Total hot run time: 14.56 s |
FE Regression Coverage ReportIncrement line coverage |
### What problem does this PR solve? Issue Number: None Related PR: apache#67186 Problem Summary: Remove unused builders, extension points, convenience APIs, duplicate validation, and generic snapshot resolvers from the HMS partition batch path. Keep the access, batch executor, and transport flow one-way; preserve exact and omission-tolerant request semantics without an exception-subtype protocol. Keep single-flight active when partition retention is disabled, while using the existing bulk-load generation fence so invalidation still forces waiters to retry. ### Release note None ### Check List (For Author) - Test: Unit Test - 183 connector-cache and HMS tests passed through the JUnit launcher. - Modified connector sources and tests passed javac and Checkstyle 10.23.0. - Full Maven tests were not run because this worktree lacks thirdparty/installed/bin/thrift and thirdparty/installed/bin/protoc. - Behavior changed: Yes (disabled partition retention still coalesces concurrent loads and now preserves invalidation fencing without retaining values) - Does this need documentation: No
### What problem does this PR solve? Issue Number: None Related PR: apache#67186 Problem Summary: A same-mode partition waiter whose keys only partially overlapped an in-flight owner inherited the owner's failure even when the failure concerned keys outside the waiter request. Retry failed shared loads unless the waiter semantics and complete claimed-key set exactly match the owner, preserving caller-local failure isolation without depending on an internal exception subtype. ### Release note None ### Check List (For Author) - Test: Unit Test - 184 connector-cache and HMS tests passed through the JUnit launcher. - Modified sources and tests passed javac, Checkstyle 10.23.0, and git diff --check. - Full Maven tests were not run because this worktree lacks thirdparty/installed/bin/thrift and thirdparty/installed/bin/protoc. - Behavior changed: Yes (partially overlapping waiters retry owner failures outside their own request) - Does this need documentation: No
|
/review |
There was a problem hiding this comment.
Review completed against PR head 4c73968. The full authoritative diff and all changed paths were rechecked, with separate cache/HMS, scan/profile, MTMV, and SPI/property sweeps. Two distinct correctness issues remain and are called out inline: batch scans can leak connector statement-scope leases, and the new HMS batch-size property has inconsistent whitespace normalization. Existing cache/HMS metrics, MTMV pin, and profile-finalizer concerns were deduplicated against live review threads. No additional user focus was provided. No builds or tests were run, per the review prompt.
There was a problem hiding this comment.
Review completed against PR head 4c73968. The full authoritative diff and all changed paths were rechecked, with separate cache/HMS, scan/profile, MTMV, and SPI/property sweeps. Two distinct correctness issues remain and are called out inline: batch scans can leak connector statement-scope leases, and the new HMS batch-size property has inconsistent whitespace normalization. Existing cache/HMS metrics, MTMV pin, and profile-finalizer concerns were deduplicated against live review threads. No additional user focus was provided. No builds or tests were run, per the review prompt.
There was a problem hiding this comment.
Review completed against PR head 4c73968. The full authoritative diff and all changed paths were rechecked, with separate cache/HMS, scan/profile, MTMV, and SPI/property sweeps. Two distinct correctness issues remain and are called out inline: batch scans can leak connector statement-scope leases, and the new HMS batch-size property has inconsistent whitespace normalization. Existing cache/HMS metrics, MTMV pin, and profile-finalizer concerns were deduplicated against live review threads. No additional user focus was provided. No builds or tests were run, per the review prompt.
| int end = Math.min(begin + batchSize, allPartitions.size()); | ||
| if (batchException.get() != null || splitAssignment.isStop()) { | ||
| break; | ||
| SubmittedTaskFinalizer profileFinalizer = new SubmittedTaskFinalizer(() -> { |
There was a problem hiding this comment.
[P1] Close the connector statement scope for batch scans. FileQueryScanNode enters this path through SplitAssignment.init() -> startSplit(), so it bypasses getSplits(), the only place that currently registers statementScope::closeAll. The session captured here can own Iceberg TableLease/TrackedTable resources, but Arrow Flight sets returnResultFromLocal=false and StatementContext.close() deliberately skips its fallback close, relying on the query-finish callback. Because this batch path never registers that callback, each deferred Flight batch query can leave its scope and leases open. Please factor the callback registration into a helper and invoke it before dispatch in both batch flavors, with a lifecycle test that verifies closure after coordinator unregister.
| } | ||
| int parsed; | ||
| try { | ||
| parsed = Integer.parseInt(value); |
There was a problem hiding this comment.
[P2] Normalize the raw batch-size property before parsing. The catalog binder trims values before binding this field, but both HiveCatalogProperties.of and HudiCatalogProperties.of then pass the original map to this constructor, where Integer.parseInt(value) is called without trimming. A quoted SQL property such as hive.hms_partitions_batch_size_per_rpc = ' 5000 ' therefore binds successfully and then throws while creating/replaying the catalog. Please trim (or pass the typed field) and add Hive/Hudi whitespace tests.
### What problem does this PR solve? Issue Number: None Related PR: apache#67186 Problem Summary: Hive and Hudi catalog property binding accepts surrounding whitespace for integer values, but HmsClientConfig reparsed the raw partition batch size without trimming it. Normalize the value before parsing so both catalog paths preserve the established typed-property behavior. ### Release note None ### Check List (For Author) - Test: Unit Test - HiveCatalogPropertiesTest, HudiCatalogPropertiesTest, and HmsPartitionBatchExecutorTest - Behavior changed: Yes (partition batch sizes with surrounding whitespace are accepted consistently) - Does this need documentation: No
|
/review |
There was a problem hiding this comment.
Reviewed the complete authoritative 49-file diff and current inline history on head 62fd254. Requesting changes for two distinct current-head issues: a P1 MTMV snapshot-consistency defect and a P2 HMS single-flight outage-amplification defect.
Critical checkpoints:
- Goal and proof: The PR largely implements batched Hive partition access, explicit exact/omission contracts, cache coordination, profile reporting, and MTMV bulk freshness. The changed tests exercise many success, fallback, validation, and concurrency cases, but do not prove the two interleavings reported inline. No tests or builds were run in this review because the authoritative review instructions prohibit them.
- Scope and clarity: The implementation is broad but organized around reusable request/result/executor abstractions and connector integration points. The remaining defects are incomplete propagation across existing parallel paths, not unrelated scope.
- Concurrency and thread safety: Per-key owner/waiter election, invalidation/currentness, waiter accounting, cache close, capacity eviction, and bulk-load lifetime were traced. MF-1 remains: shape-incompatible waiters retry request-independent terminal failures and Error, multiplying work during an outage. No deadlock or separate currentness defect survived review.
- Lifecycle: Pooled/unpooled client destruction, primary-exception preservation, scan finalizers, provider reselection, and read-transaction ownership otherwise survived. The asynchronous statement-scope closure defect is already covered by current-head comment 3921057327 and is not duplicated here.
- Configuration: The new catalog batch-size property has a stable default, trimmed positive-integer validation, and consistent Hive/Hudi construction. It is catalog-scoped rather than dynamically mutable; no separate replay/configuration issue survived.
- Compatibility: Exact and omission-tolerant APIs preserve ordered scalar fallback for connectors. Existing connector-SPI version/surface concerns are already covered by comments 3868649356 and 3870178439. No FE-BE protocol or storage-format change is introduced.
- Parallel paths and conditions: Exact versus omission callers, synchronous/batch/streaming scan modes, PCT versus non-PCT MTMV bases, SELF_MANAGE/mixed MVs, and local/cloud version paths were checked. MF-2 is the remaining missed parallel path: non-PCT table snapshots bypass the task pin.
- Test coverage and results: Focused FE unit tests use deterministic coordination for the covered races, but coverage is missing for broad-owner/singleton-waiter global failure and non-PCT S1-to-S2 task persistence. Existing changed expectations were inspected; no result files were added.
- Observability: Query-profile and batch-stat success/failure handoff is generally preserved. The reachable exact-waiter stats gap has no current production stats consumer and was dismissed with evidence; existing profile/metrics concerns were duplicate-fenced.
- Persistence and data correctness: MF-2 can persist S2 freshness after refresh SQL read S1, allowing stale MV contents into rewrite. Other mapping/PCT pin and missing-result isolation paths now preserve their intended snapshot/error contracts. There is no new transaction, data-write, EditLog, or FE-BE variable-passing path to validate.
- Performance: Batching removes the principal per-partition RPC pattern, but MF-1 can undo single-flight protection during global outages. No additional substantiated hot-path regression survived the full sweep.
- Other issues and user focus: No additional review focus was specified. All other candidates were independently accepted, dismissed with code evidence, or fenced by existing threads; three Round 2 reviewers returned NO_NEW_VALUABLE_FINDINGS.
| return context; | ||
| } | ||
|
|
||
| private Optional<MvccSnapshot> resolveSnapshot(MTMVRelatedTableIf table) { |
There was a problem hiding this comment.
[P1] Apply the task pin to non-PCT table snapshots too. This resolver now keeps mapping and PCT partition snapshots at S1, but MTMVPartitionUtil.getTableSnapshotFromContext still calls MvccUtil.getSnapshotFromContext directly. During MTMVTask.run the outer context has no StatementContext, so an Iceberg/Paimon non-PCT base can materialize latest S2 here; each later refresh SQL explicitly scans the task pin S1, while generatePartitionSnapshots persists the cached S2. The MV can then be considered fresh at S2 and used for rewrite although S2 was never materialized. Reuse this context resolver for table-level snapshots and add a non-PCT S1-to-S2 task interleaving test.
| try { | ||
| Throwable failure = batch.await(); | ||
| if (failure != null) { | ||
| if (batch.allowMissing != allowMissing |
There was a problem hiding this comment.
[P2] Share request-independent terminal failures with partial waiters. This compatibility check runs before the failure is classified, so a waiter covering a subset (or using the other missing-result contract) retries even pool/client-creation, authentication, connection failures, and Error. For one owner of N cold keys plus N singleton waiters, a global owner failure releases every waiter into a separate delegate attempt under the same outage, defeating single-flight and potentially saturating the metadata pool. Only result-integrity failures can depend on identities outside a waiter's request; propagate request-independent failures and Error to all overlapping waiters, and add a broad-owner/singleton-waiters outage test.
### What problem does this PR solve? Issue Number: None Related PR: apache#67186 Problem Summary: Partial single-flight waiters retried request-independent HMS failures, amplifying metadata outages, while MTMV non-PCT table snapshots bypassed the refresh task's pinned MVCC snapshot. Share terminal HMS failures with all overlapping waiters while retaining retries for request-dependent result-integrity failures, and route non-PCT table snapshot reads through the refresh context's pin resolver. ### Release note None ### Check List (For Author) - Test: Unit Test - CachingHmsClientTest and MTMVRefreshContextBatchTest - Behavior changed: Yes (overlapping HMS failures remain single-flight and MTMV refresh snapshots remain task-pinned) - Does this need documentation: No
|
/review |
There was a problem hiding this comment.
Reviewed the full authoritative PR diff and the required connector/FE guidance. The cache, batching/transport, profile publication, Hive/Hudi integration, and MTMV snapshot paths were checked with three independent risk sweeps. One distinct P2 lifecycle issue is raised inline below. The SPI-major, MVCC mapping/type, vanished-partition, profile-finalization, and request-failure concerns were hard-deduplicated against existing review threads. No builds or tests were run because the review bundle forbids them.
| return execute(client -> { | ||
| List<Partition> partitions = | ||
| client.getPartitionsByNames(dbName, tableName, partNames); | ||
| return partitions.stream() |
There was a problem hiding this comment.
[P2] Preserve the closed-client guard for empty partition requests. getPartitionsWithStats and getExistingPartitionsWithStats build the request before any closed check, while HmsPartitionBatchExecutor returns immediately for an empty request (lines 57-65), so after ThriftHmsClient.close() an empty getPartitions/getExistingPartitions call silently succeeds. The old getPartitions path entered execute, which rejected closed clients up front; this now lets a close/query race look like a valid empty scan. Check closed at the start of both public batch methods (and add pooled/unpooled post-close empty-request coverage) to preserve the client lifecycle contract.
924060929
left a comment
924060929
left a comment
There was a problem hiding this comment.
Re-reviewed current head 0478f0c35cf7be5a45e8878a0cd06dc15f56a0c9.
The core layering is now much clearer: one logical HmsClient request flows through a bounded batch executor to a leaf HMS transport, with response validation and request-order reconstruction centralized in the executor. The MTMV integration is also now engine-agnostic through the default bulk MTMVRelatedTableIf#getPartitionSnapshots adapter.
Three current issues still need to be addressed before approval:
- The batch/streaming
startSplitpaths still bypass the query-finish registration used bygetSplits, so the connector statement scope can remain open for deferred/Arrow Flight scans. This is the remaining P1 lifecycle issue. - A successful pruning HMS request can still disappear from Query Profile when later Nereids pruning reduces the selection to zero, because the stats remain on the handle and
planScannever consumes them. - Empty partition requests on a closed
ThriftHmsClientstill return successfully because the executor short-circuits before the closed-client guard.
Several older unresolved threads are already fixed in the current code: selective nonempty pruning now carries stats on the handle, whole-table freshness uses omission-tolerant access, per-partition MTMV rewrite failures are deferred and isolated, and failed pruning stats are collected through statement scope. The removed fallback-timeout thread is also obsolete.
The PR is still broader than its stated scope: CachingHmsClient implements a substantial per-partition single-flight owner/waiter protocol and extends generic MetaCache currentness behavior even though the PR description says cache single-flight is out of scope. This should either be split into a separately justified change or described and validated as part of this PR.
Finally, the PR currently conflicts with master in MTMV, MTMVTask, and an MTMV rewrite test. This is not a mechanical rebase because master has substantially refactored MTMV around IVM refresh/fallback flows; the preload and pinned-snapshot placement needs another lifecycle review after resolving those conflicts.
Recommendation: keep CHANGES_REQUESTED until the P1/P2 items are fixed and the rebased MTMV flow is re-reviewed. The request-shape evidence is useful, but the current head still has no real 120k-partition end-to-end performance result.
3 participants
What problem does this PR solve?
Issue Number: None
Related PR: None
Problem Summary:
Hive tables with very large partition counts could either issue one HMS partition-object RPC per partition on legacy caller paths or send every partition name in one unbounded
getPartitionsByNamesrequest. The first form creates excessive serial RPC latency; the second risks Thrift/HMS message limits and large temporary allocations.This PR narrows the change to the shared HMS partition-object boundary. Callers continue to submit one logical partition-name list through
HmsClient#getPartitions; the existing cache aggregates misses, one HMS batch executor owns bounded chunking, adaptive fallback and strict response validation, and a leaf transport performs onegetPartitionsByNamesinvocation per physical attempt. Query, statistics, write and Hive-backed MTMV callers therefore receive the same batching behavior without implementing their own chunk/retry loops.Common HMS batch execution
hive.hms_partitions_batch_size_per_rpcbounds each physical partition-object request; the default is 5,000.hive.metastore.limit.partition.request/ “partitions scanned ... exceeds limit” failure is recognized.hive.metastore.client.pool.size=0, successful chunks in one logical request reuse one temporary HMS client. A failed physical call taints and destroys that client before a fallback attempt creates another.HmsClientConfig.Strict result integrity
Narrow MTMV bulk adapter
MTMVRelatedTableIf#getPartitionSnapshotshas a compatibility default that retains the existing scalar loop for non-bulk table implementations.HmsClient#getPartitionscall; the common executor then splits it into bounded physical requests.MTMVRefreshContextkeeps only a request-scoped table → partition → snapshot cache. It unions mapped base partitions before the existing loops in sync, need-refresh, display, persistence and rewrite paths.MTMVTaskpreloads the complete need-refresh union before splitting execution groups, so the default one-partition group size cannot regress first/manual/COMPLETE refreshes to singleton HMS requests.With the default batch size, a cold 120,000-partition logical object request becomes 24 bounded requests instead of one 120,000-name request. A 160,000-partition Hive-backed MTMV union becomes one logical bulk load and 32 bounded physical requests, rather than one object request per mapped partition.
Query Profile observability
Connector Metadata Accessprofile through the existingConnectorScanProfilehook.HmsPartitionRequest.Scope boundaries:
SplitSourcelifecycle behavior are unchanged.Release note
Hive Metastore partition-object access now uses configurable bounded RPC batches, strict response validation, and adaptive fallback for explicit oversized-request failures. Hive-backed MTMV partition freshness is aggregated into bulk logical requests before HMS batching. Hive Query Profile also shows the resulting partition-batch request shape and elapsed time.
Deterministic request-shape evidence
5000 → 2500 → 1250 → 625, then all objects completeThese rows describe deterministic orchestration and request shape; they are not a substitute for a real 120,000-partition HMS end-to-end rerun.
Validation
Latest review increment: 13 HMS batch-executor tests and 20 PluginDriven scan batch/profile tests passed; Hive/Hudi catalog-property tests also passed.
111 focused FE-core tests passed: MTMV refresh context, partition utilities, rewrite, task, and plugin-driven MVCC table paths.
72 focused connector tests passed: HMS batching/cache/Thrift integration, Hive freshness, and connector SPI surface.
The final no-cache 60-module Maven
validatereactor passed with zero Checkstyle violations.git diff --checkpassed.Effective PR diff against its master base: 43 files, 3,086 additions and 207 deletions, excluding the uncommitted design/review documents.
Three independent final review scopes converged with no new P1/P2 findings after fixing task preloading, pool-disabled client reuse, and Hive's standard partition-limit classifier.
Focused Maven compilation/tests reused the worktree's existing generated sources because
thirdparty/installedis absent; no successful full./build.sh --ferun is claimed.